BIgtable: 11. Implement ReadRows retries #2986

igorbernstein2 · 2018-03-02T04:12:38Z

No description provided.

google-cloud-bigtable/pom.xml

+    <dependency>
+      <groupId>io.grpc</groupId>
+      <artifactId>grpc-testing</artifactId>
+    </dependency>


kevinsi4508

more to come.

...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java

@@ -75,6 +79,15 @@ public static EnhancedBigtableStub create(EnhancedBigtableStubSettings settings)
            .setEndpoint(settings.getEndpoint())
            .setCredentialsProvider(settings.getCredentialsProvider());

+    // ReadRow retries are handled in the overlay: disable retries in the base layer (but make


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+
+/**
+ * An implementation of a {@link StreamResumptionStrategy} for merged rows. This class tracks the
+ * last complete row seen and upon retry can build a request to resume the stream from were it left


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+    implements StreamResumptionStrategy<ReadRowsRequest, RowT> {
+  private final RowAdapter<RowT> rowAdapter;
+  private ByteString lastKey = ByteString.EMPTY;
+  private long numProcessed;


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+  }
+
+  @Override
+  public ReadRowsRequest getResumeRequest(ReadRowsRequest request) {


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+
+  @Override
+  public ReadRowsRequest getResumeRequest(ReadRowsRequest request) {
+    if (lastKey.isEmpty()) {


...oud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/package-info.java

+ *       rows. The logical row representation is configurable via a RowAdapter. Please note that
+ *       this will also emit special marker rows that help with retries in the next stage, but need
+ *       to be filtered out.
+ *   <li>ReadRowsTracker (+ helpers): Implements resuming retries for gax's Callables#retrying


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+    if (request.getRowsLimit() > 0) {
+      Preconditions.checkState(
+          request.getRowsLimit() >= numProcessed,
+          "Detected too many responses for the current row limit during a retry.");


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+      Preconditions.checkState(
+          request.getRowsLimit() >= numProcessed,
+          "Detected too many responses for the current row limit during a retry.");
+      builder.setRowsLimit(request.getRowsLimit() - numProcessed);


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+    if (request.getRows().getRowKeysList().isEmpty()
+        && request.getRows().getRowRangesList().isEmpty()) {
+      newRowSet.addRowRanges(RowRange.newBuilder().setStartKeyOpen(lastKey).build());
+    } else {


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+      }
+
+      for (RowRange rowRange : request.getRows().getRowRangesList()) {
+        RowRange.Builder rangeBuilder = RowRange.newBuilder();


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+            }
+            break;
+          case ENDKEY_NOT_SET:
+            rangeBuilder.clearEndKey();


...igtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsRetryTest.java

+
+  @After
+  public void tearDown() throws Exception {
+    if (client != null) {


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+      Preconditions.checkState(
+          request.getRowsLimit() >= numProcessed,
+          "Detected too many responses for the current row limit during a retry.");
+      builder.setRowsLimit(request.getRowsLimit() - numProcessed);


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+    // synthetic row marker is emitted when the server has read a lot of data that was filtered out.
+    // So it can trim the start of the scan, but does not contribute to the row limit.
+    lastKey = rowAdapter.getKey(response);
+    if (!rowAdapter.isScanMarkerRow(response)) {


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+    // Special case: empty query implies full table scan
+    if (request.getRows().getRowKeysList().isEmpty()
+        && request.getRows().getRowRangesList().isEmpty()) {
+      newRowSet.addRowRanges(RowRange.newBuilder().setStartKeyOpen(lastKey).build());


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+    } else {
+      for (ByteString key : request.getRows().getRowKeysList()) {
+        if (ByteStringComparator.INSTANCE.compare(key, lastKey) > 0) {
+          newRowSet.addRowKeys(key);


...igtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsRetryTest.java

+import org.mockito.runners.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class ReadRowsRetryTest {


igorbernstein2 · 2018-03-05T14:13:53Z

All feedback should be addressed. PTAL

igorbernstein2 · 2018-03-05T15:55:30Z

Thanks for reviewing! And also, really good catch on retrying a fulfilled request bug!

All feedback should be addressed, PTAL

igorbernstein2 · 2018-03-05T17:36:32Z

@garrettjonesgoogle PTAL when you have a moment. If it looks ok, please merge

...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+    Builder builder = request.toBuilder();
+
+    if (request.getRowsLimit() > 0) {
+      // NOTE: the edge case of request.getRowsLimit() == numProcessed is handled below


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+    if (request.getRows().getRowKeysList().isEmpty()
+        && request.getRows().getRowRangesList().isEmpty()) {
+
+      request =


igorbernstein2 · 2018-03-05T19:49:19Z

All feedback should be addressed, PTAL

...ain/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsRetryCompletedCallable.java

+ * an OK response.
+ *
+ * <p>This callable works in tandem with {@link ReadRowsResumptionStrategy}, which will send a
+ * {@link #FULFILLED_REQUEST_MARKER} to be processed by this callable. Which will promptly notify


...oud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/package-info.java

+ * Implementation details for {@link
+ * com.google.cloud.bigtable.data.v2.stub.EnhancedBigtableStub#readRowsCallable()}.
+ *
+ * <p>The ReadRows protocol is optimized for transmission and cannot be consumed directly. This


...oud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/package-info.java

+ *       to be filtered out.
+ *   <li>RowMerger (+ helpers): Implements resuming retries for gax's Callables#retrying middleware.
+ *   <li>FilterMarkerRowsCallable: Filters out marker rows that are used for efficient retry
+ *       resumes. This is necessary because


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+  public void onProgress(RowT response) {
+    // Last key can come from both the last processed row key and a synthetic row marker. The
+    // synthetic row marker is emitted when the server has read a lot of data that was filtered out.
+    // So it can trim the start of the scan, but does not contribute to the row limit.


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+  @Override
+  public ReadRowsRequest getResumeRequest(ReadRowsRequest request) {
+    // An empty lastKey means that we have not successfully read the first row,
+    // resume with the original request object.


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+
+    ReadRowsRequest originalRequest = request;
+
+    // Special case: empty query implies full table scan, make this explicit by adding an unbounded


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+            continue;
+          } else {
+            rowRangeBuilder.setEndKeyClosed(rowRange.getEndKeyClosed());
+          }


...rc/main/java/com/google/cloud/bigtable/data/v2/stub/readrows/ReadRowsResumptionStrategy.java

+    }
+
+    // Edge case: retrying a fulfilled request.
+    // A fulfilled request is one that has had all of its row keys and ranges fulfilled or if it had


...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java

@@ -75,6 +80,15 @@ public static EnhancedBigtableStub create(EnhancedBigtableStubSettings settings)
            .setEndpoint(settings.getEndpoint())
            .setCredentialsProvider(settings.getCredentialsProvider());

+    // ReadRow retries are handled in the overlay: disable retries in the base layer (but make
+    // sure to preserve the exception callable settings.


…al variable for request munging

igorbernstein2 · 2018-03-06T18:01:24Z

Rebased & incorporated all feedback. PTAL, if it looks ok, please merge

garrettjonesgoogle · 2018-03-06T18:38:55Z

LGTM

igorbernstein2 requested a review from pongad as a code owner March 2, 2018 04:12

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 2, 2018

garrettjonesgoogle reviewed Mar 2, 2018

View reviewed changes

google-cloud-bigtable/pom.xml Outdated

<dependency>

<groupId>io.grpc</groupId>

<artifactId>grpc-testing</artifactId>

</dependency>

This comment was marked as spam.

Sign in to view

googleapis deleted a comment Mar 2, 2018

kevinsi4508 reviewed Mar 2, 2018

View reviewed changes

kevinsi4508 reviewed Mar 5, 2018

View reviewed changes

googleapis deleted a comment Mar 5, 2018

kevinsi4508 reviewed Mar 5, 2018

View reviewed changes

googleapis deleted a comment Mar 5, 2018

googleapis deleted a comment Mar 6, 2018

garrettjonesgoogle reviewed Mar 6, 2018

View reviewed changes

igorbernstein2 added 4 commits March 6, 2018 12:46

BIgtable: 11. Implement ReadRows retries

351aae9

add default case

fd8ced6

fix scope for testing dep

d1b8d8a

codacy fixes

afe9321

igorbernstein2 added 4 commits March 6, 2018 12:46

address feedback

d8887c4

handle retries of fulfilled requests

d0aa23a

re-organize code: move row limit setting after the edge case, use loc…

a8c75c3

…al variable for request munging

address feedback

92b4a71

igorbernstein2 force-pushed the 11-impl-readrows-retries branch from 7682c94 to 92b4a71 Compare March 6, 2018 17:58

fix javadoc link

003d88e

garrettjonesgoogle merged commit b87a5ee into googleapis:master Mar 6, 2018

igorbernstein2 deleted the 11-impl-readrows-retries branch March 8, 2018 02:59

igorbernstein2 mentioned this pull request Mar 29, 2018

Bigtable: Implement query sharding by generalizing ReadRows resume request builder. #3103

Merged


		ReadRowsRequest originalRequest = request;

		// Special case: empty query implies full table scan, make this explicit by adding an unbounded

BIgtable: 11. Implement ReadRows retries #2986

BIgtable: 11. Implement ReadRows retries #2986

Conversation

igorbernstein2 commented Mar 2, 2018

This comment was marked as spam.

kevinsi4508 left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

igorbernstein2 commented Mar 5, 2018

igorbernstein2 commented Mar 5, 2018

igorbernstein2 commented Mar 5, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

igorbernstein2 commented Mar 5, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

igorbernstein2 commented Mar 6, 2018

garrettjonesgoogle commented Mar 6, 2018